-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ACL]: Optimize DASH ACL by introducing cluster tag #336
Conversation
176c305
to
d0594b6
Compare
Signed-off-by: Ze Gan <ganze718@gmail.com>
d0594b6
to
e62ac0a
Compare
4e27db6
to
43f33c3
Compare
Signed-off-by: Ze Gan <ganze718@gmail.com>
46c152a
to
b303111
Compare
Signed-off-by: Ze Gan <ganze718@gmail.com>
b303111
to
a245bf5
Compare
Signed-off-by: Ze Gan <ganze718@gmail.com>
@@ -311,8 +321,10 @@ DASH_ACL_RULE_TABLE:{{group_id}}:{{rule_num}} | |||
"action": {{action}} | |||
"terminating": {{bool}} | |||
"protocol": {{list of protocols}} | |||
"src_addr": {{list of address}} | |||
"dst_addr": {{list of address}} | |||
"src_tag": {{list of tag name}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark this optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have an explicit "optional" mark to "tag" field? IMHO, most of fields in DASH ACL RULE TABLE are optional. E.G. We can only set src_tag or dst_port and set other fields to be empty. So, should we also mark them?
1b1e1d4
to
e1ebff0
Compare
@@ -29,6 +30,8 @@ match_kind { | |||
table table_name { \ | |||
key = { \ | |||
meta. ## table_name ##_dash_acl_group_id : exact @name("meta.dash_acl_group_id:dash_acl_group_id"); \ | |||
meta.dst_tag_map : ternary @name("meta.dst_tag_map:dst_tag"); \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
match type: optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is here optional?
- We would like a mask operation.
E.G. If the source IP of packet belong to tag 1 and 2, and this ACL rule is applied to tag 2 and 3, it should be matched. If another ACL rule is applied to tag 3 and 4, it should be mismatched. - If the tag was not provided, the target rule could match all tags.
I'm not sure whether the optional match_kind can meet above behaviors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A case is if a prefix belongs to tags, A, B, C, D (1111) and an ACL rule is applied to tags A, C(1010).
If my understanding is correct, to the match_kind, optional. the mask should be 1111, which implies tag (1111) & mask(1111) != target_tag(1010)
.
But to the match_kind, ternary, the mask will be 1010. So the rule will be matched due to tag(1111) & mask(1010) = target_tag(1010)
.
@name("src_tag|dash_tag") | ||
table src_tag { | ||
key = { | ||
meta.src_ip_addr : lpm @name("meta.src_ip_addr:sip"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
match type: prefix_list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here shouldn't be a list. AFAIK, one prefix can belong to multiple clusters with different tags, So the prefix list will be separated into the tag table. Otherwise if it's prefix_list, it's hard to solve the following case:
prefix(A, B, C)=>tag1
prefix(C,D,E)=>tag2
Because we have to dynamically create a new entry for prefix(C)=>tag1, tag2. And if the C was updated, we need also update all entries that include prefix(C).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you have a scenario where eg. 10.0.0.0/8 -> tag1, 10.1.0.0/16 -> tag2.. How do you expect lpm match here to resolve and assign tags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assumption of 1 prefix matching and 1 tag assignment. What if there are multiple prefixes matching to 1 tag (overlapping prefixes matching to 1 tag)? (A tag is simply a list of prefixes).
Tags will be in the Control Plane and could be in the Data Plane. Whether or not this tag is represented in the Fastpath is an 'implementation choice'. If a Data Plane chooses to enumerate, for example.
Assumption is that it would be pushed to the SouthBound.
Tag is just a name for a list to be pushed.
Indirection for the Control Plane to use.
@prsunny - we do want to optimize from the Data Plane side as well. Scale limits are in the HLD, however this is possible.
@vincent-xs suggests the API is flexible enough for the supplier to control
@jfingerh - remember that the bit-factor must be big enough :) but we do have scale
Signed-off-by: Ze Gan <ganze718@gmail.com>
e1ebff0
to
8a1ef51
Compare
"src_port": {{list of range of ports}} | ||
"dst_port": {{list of range of ports}} | ||
"protocol": {{list of protocols}} (OPTIONAL) | ||
"src_tag": {{list of tag name}} (OPTIONAL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specifying a src_tag with empty prefix must not match packets (except if src_addr is also specified). When specifying a src_tag and src_prefix, it shall be a OR. Please clarify it in documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specifying a src_tag with empty prefix must not match packets.
I add this description under the TAG table.When specifying a src_tag and src_prefix, it shall be a OR. Please clarify it in documentation
. I think this should be an AND instead of OR. If you want a OR, just to define another rule.
There are three reasons:- The tag fields are same as other fields like protocol, port and so on. The relationship between src, protocol and port should be AND.
- The TAG is for identifying a packets that belongs to a specific group. In the further, maybe we can identify a packets not only from address but also from other features.
- I think if you want to implement
OR
in P4, we have to separate the TAG and ADDR to different ACL tables, which will increase the complexity of the ACL system.
If you accept my points, I can polish and clarify the descriptions for TAG.
Signed-off-by: Ze Gan <ganze718@gmail.com>
b9019e7
to
b84c4ef
Compare
@mukeshmv could you please add your scale related comments/questions to this review. Thanks! |
|
||
|
||
``` | ||
DASH_PREFIX_TAG_TABLE:{{tag_name}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add some scale numbers for the ACL tags in the HLD ?
- How many total tags ?
- How many prefixes per tag ?
- How many tags can each prefix belong to ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to update the HLD with the scale numbers.
Possibly by sometime this week 3/16/2023
@prsunny
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the scale numbers of tags in the above scale table.
@prsunny - may we have an update re: this one? There will be work needed in SAI. |
Signed-off-by: Ze Gan <ganze718@gmail.com>
Signed-off-by: Ze Gan <ganze718@gmail.com>
Signed-off-by: Ze Gan <ganze718@gmail.com>
No need to generated separate APIs for SRC/DST tags. Can be joined together like ACL APIs. |
"protocol": {{list of protocols}} (OPTIONAL) | ||
"src_tag": {{list of tag name}} (OPTIONAL) | ||
"dst_tag": {{list of tag name}} (OPTIONAL) | ||
"src_addr": {{list of prefix}} (OPTIONAL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the documentation switched from address to prefix (prefix is definitely what we would expect).
BUT the P4 model is still generating IP address, not prefix. could the P4 code be fixed to match the documentation?
/**
* @brief Optional matched key dip
*
* @type sai_ip_address_t
* @flags MANDATORY_ON_CREATE | CREATE_ONLY
*/
SAI_DASH_ACL_RULE_ATTR_DIP,
/**
* @brief Optional matched key sip
*
* @type sai_ip_address_t
* @flags MANDATORY_ON_CREATE | CREATE_ONLY
*/
SAI_DASH_ACL_RULE_ATTR_SIP,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried to manually define the DASH_MATCH in the file, dash_acl.p4?
#define DASH_MATCH // Please explicitly define this line in dash-pipeline/bmv2/dash_acl.p4
#ifdef DASH_MATCH
#define LIST_MATCH list
#define RANGE_LIST_MATCH range_list
#else
#ifdef TARGET_BMV2_V1MODEL
#define LIST_MATCH optional
#define RANGE_LIST_MATCH optional
#endif // TARGET_BMV2_V1MODEL
#ifdef TARGET_DPDK_PNA
#define LIST_MATCH ternary
#define RANGE_LIST_MATCH range
#endif // TARGET_DPDK_PNA
#endif
If you define it, you will get the correct SAI header.
/**
* @brief List matched key dip
*
* @type sai_ip_prefix_list_t
* @flags MANDATORY_ON_CREATE | CREATE_ONLY
*/
SAI_DASH_ACL_RULE_ATTR_DIP,
/**
* @brief List matched key sip
*
* @type sai_ip_prefix_list_t
* @flags MANDATORY_ON_CREATE | CREATE_ONLY
*/
SAI_DASH_ACL_RULE_ATTR_SIP,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is that something that could be taken care of so we align things up?
thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vincent-xs will look at SAI Master for generation code
Difference between APIs for SAI and bmv2 defined match types for SIP and DIP
By default not-enabled (hidden compile to check for possibly)?
@Pterosaur possibly can also check the 'define'
Hi @marian-pritsak , the APIs were automatically generated from P4.
If you have any solution, I'm very glad to learn it. |
@name("src_tag|dash_tag") | ||
table src_tag { | ||
key = { | ||
meta.src_ip_addr : lpm @name("meta.src_ip_addr:sip"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you have a scenario where eg. 10.0.0.0/8 -> tag1, 10.1.0.0/16 -> tag2.. How do you expect lpm match here to resolve and assign tags?
dst_addr = list of destination ip prefixes ',' separated | ||
src_port = list of range of source ports ',' separated | ||
dst_port = list of range of destination ports ',' separated | ||
src_tag = list of source tag name ',' separated; if not provided, match on all source TAGs or no TAG. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does it mean by "match on all source TAGs or no TAG" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Intention is if the attribute is not provided, it is like any other ACL rule. If not provided, wildcard match. Match on ANY tag or NO tag (reword)?
@Pterosaur @prsunny
If packet matches /8, it will be tagged with tag1. It's not LPM match, will verify with @Pterosaur
@svshah-intel one prefix matching to multiple tags can result in multiple matches - is this the question? Comments are about lookup and actions.
@Pterosaur You need to keep two tables in P4 code, but unite them under one API name, like in the example you gave with this annotation |
@Pterosaur , can you please resolve conflict? |
@Pterosaur after considering this, I think it's better to generate separate APIs for src and dst tags, which will allow implementations keep two different LPM tables for a better utilization of resources. |
dst_addr = list of destination ip prefixes ',' separated | ||
src_port = list of range of source ports ',' separated | ||
dst_port = list of range of destination ports ',' separated | ||
src_tag = list of source tag name ',' separated; if not provided, match on all source TAGs or no TAG. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Pterosaur and @prsunny
Instead of this: 'match on all source TAGs or no TAG'
Could we have a rewording to: 'match on ANY tag or NO tag'?
Per Community conversation, the intention is that if the attribute is not provided, it is like any other ACL rule. If not provided, it will be a wildcard match. Could we have it reworded similar to the above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will submit a PR to polish it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change it in PR: #359
No description provided.